Skip to content

fix circlemarker by updating equality check#605

Closed
Falkyouall wants to merge 1 commit intoPaulLeCam:masterfrom
Falkyouall:master
Closed

fix circlemarker by updating equality check#605
Falkyouall wants to merge 1 commit intoPaulLeCam:masterfrom
Falkyouall:master

Conversation

@Falkyouall
Copy link
Copy Markdown

@Falkyouall Falkyouall commented Jul 23, 2019

After a 2 years old discussion about this issue which tracks back to your repository, i propose a little fix that makes a lot of lifes easier.

The base of the fix lies in the CircleMarker.js file, which until now, made it impossible to render Circlemarkers inside of react-leaflet-markercluster without throwing:

TypeError: Cannot read property 'lat' of undefined.

This fix changes the equality check for the updateLeafletElement center property.

- if (toProps.center !== fromProps.center)
+ if (!latLng(toProps.center).equals(fromProps.center))

@Falkyouall Falkyouall changed the title fix circlemarker update equality check fix circlemarker by updating equality check Jul 23, 2019
@PaulLeCam
Copy link
Copy Markdown
Owner

Hi, thanks for your PR but this should be part of your app or plugin logic.
As you can see in the CircleMarker documentation the center prop is required, if you set it to undefined as raised in the error message you quoted, it's not expected to work.

@PaulLeCam PaulLeCam closed this Jul 29, 2019
@Falkyouall
Copy link
Copy Markdown
Author

Falkyouall commented Jul 29, 2019

Hi Paul,
Since im providing CircleMarker center props its rather the third party plugin. I checked the react-markercluster library, and i don't confirm any bug or mistake in there, but if you do have a idea about this issue, here is the stacktrace:

project | @ | leaflet-src.js:1612
latLngToPoint | @ | leaflet-src.js:1452
project | @ | leaflet-src.js:3834
_removeFromGridUnclustered | @ | leaflet.markercluster.js:6
_removeLayer | @ | leaflet.markercluster.js:6
removeLayer | @ | leaflet.markercluster.js:6
_moveChild | @ | leaflet.markercluster.js:6
_childMarkerMoved | @ | leaflet.markercluster.js:6
fire | @ | leaflet-src.js:588
setLatLng | @ | leaflet-src.js:7654
updateLeafletElement | @ | CircleMarker.js:31
componentDidUpdate | @ | MapLayer.js:50
componentDidUpdate | @ | Path.js:33

I don't expect you to dig into this, would greatly appreciate it tho.

@luiz-chagas
Copy link
Copy Markdown

I am running the exact same issue with the same stack trace.
For now I'm using a patched forked version of this package, hopefully the author will reconsider this PR.

@PaulLeCam
Copy link
Copy Markdown
Owner

Sorry I don't have time to investigate issues in third-party plugins.
As I wrote in the previous comment, the error seems to indicate the provided center prop is undefined, so I'd suggest you start by trying to figure out why your app or plugin is providing an undefined position and fix the problem there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants